ZOOKEEPER-5037: Add export and import commands to the CLI#2375
ZOOKEEPER-5037: Add export and import commands to the CLI#2375ckrumbein wants to merge 1 commit intoapache:masterfrom
Conversation
PDavid
left a comment
PDavid
left a comment
There was a problem hiding this comment.
Many thanks for this contribution. These commands work really nicely. 👍
Added some minor comments / ideas.
| */ | ||
| public class ExportCommand extends CliCommand { | ||
|
|
||
| private static Options options = new Options(); |
There was a problem hiding this comment.
This field can be changed to be final.
| } catch (IllegalArgumentException ex) { | ||
| throw new MalformedPathException(ex.getMessage()); | ||
| } catch (KeeperException|InterruptedException ex) { | ||
| throw new CliException(ex); |
There was a problem hiding this comment.
I think we should rather use CliWrapperException here instead of CliException because CliWrapperException provides user-friendly error messages. Other commands are using this.
For example if znode does not exits now:
[zk: 127.0.0.1:2181(CONNECTED) 1] export /zookeeper/a my_data
org.apache.zookeeper.KeeperException$NoNodeException: KeeperErrorCode = NoNode for /zookeeper/a
[zk: 127.0.0.1:2181(CONNECTED) 2]
using CliWrapperException:
[zk: 127.0.0.1:2181(CONNECTED) 0] export /zookeeper/a my_data
Node does not exist: /zookeeper/a
[zk: 127.0.0.1:2181(CONNECTED) 1
| throw new CliException(ex); | |
| throw new CliWrapperException(ex); |
| } | ||
| data = (data == null) ? "null".getBytes() : data; | ||
| try { | ||
| Files.write(Paths.get(filepath), data, StandardOpenOption.CREATE); |
There was a problem hiding this comment.
What should happen if the file already exists on disk?
At the moment with only StandardOpenOption.CREATE will append to an existing file rather than replacing it.
If you export a znode twice to the same file, you get the data concatenated. Or if you export a znode to an existing file which has longer content, it will just replace some amount of the file content from the start.
Should we rather overwrite the file? If yes, we chould use CREATE + TRUNCATE_EXISTING, or simply use the default Files.write(path, data) which creates/truncates by default.
| } | ||
|
|
||
| public ImportCommand() { | ||
| super("import", "[-s] [-v version] path filepath"); |
There was a problem hiding this comment.
For both commands we could use the 3 arg constructor:
| super("import", "[-s] [-v version] path filepath"); | |
| super("import", "[-s] [-v version] path filepath", options); |
This would include the formatted option descriptions in the generated help output:
zk: 127.0.0.1:2181(CONNECTED) 1] import
import [-s] [-v version] path filepath
-s stats
-v <arg> version
[zk: 127.0.0.1:2181(CONNECTED) 2] export
export [-s] [-w] path filepath
-s stats
-w watch
This is what we have as in this PR:
[zk: 127.0.0.1:2181(CONNECTED) 0] import
import [-s] [-v version] path filepath
[zk: 127.0.0.1:2181(CONNECTED) 1] export
export [-s] [-w] path filepath
[zk: 127.0.0.1:2181(CONNECTED) 2
| deleteall path | ||
| delquota [-n|-b|-N|-B] path | ||
| exit | ||
| export path filepath |
There was a problem hiding this comment.
Nice that you extended the documentation! 👍
Should we also include the options like so?
| export path filepath | |
| export [-s] [-w] path filepath |
| getAllChildrenNumber path | ||
| getEphemerals path | ||
| history | ||
| import path filepath |
There was a problem hiding this comment.
and here:
| import path filepath | |
| import [-s] [-v version] path filepath |
| Download the contents of a znode to an external file | ||
|
|
||
| ```bash | ||
| [zkshell: 1] export /zookeeper/config path/to/config.txt |
There was a problem hiding this comment.
Maybe we could add here what the options are doing, like what we have for get here:
https://github.com/apache/zookeeper/blob/master/zookeeper-docs/src/main/resources/markdown/zookeeperCLI.md?plain=1#L206
|
Checkstyle identified some formatting problems in the build: https://ci-hadoop.apache.org/job/zookeeper-precommit-github-pr/job/PR-2375/1/console |
2 participants
This PR adds two commands to the ZooKeeper CLI, export and import:
My team has been using both of these commands for some time. We are confident they are solid.